feat(sedona-gdal): add dataset and vector/raster wrappers#699
feat(sedona-gdal): add dataset and vector/raster wrappers#699Kontinuation wants to merge 8 commits intoapache:mainfrom
Conversation
Kontinuation
force-pushed
the
feat/sedona-gdal-safe-core
branch
7 times, most recently
from
1c53d55 to
efb5606
Compare
Kontinuation
force-pushed
the
feat/sedona-gdal-safe-core
branch
from
efb5606 to
bfda063
Compare
There was a problem hiding this comment.
Pull request overview
This PR expands sedona-gdal’s safe wrapper layer by adding core dataset/driver abstractions plus initial vector (layer/feature) and raster (raster band) wrappers, forming the foundation for higher-level raster/vector operations.
Changes:
- Add
DatasetandDriverwrappers (open/create/copy, spatial ref, geo-transform, layer creation). - Add vector wrappers for
Layer/Featureand raster wrapper forRasterBand(+ helpers/tests). - Minor test cleanup and new error variant for raster buffer size validation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| c/sedona-gdal/src/vsi.rs | Test assertion tweak for empty buffer representation. |
| c/sedona-gdal/src/vector/layer.rs | New OGR layer wrapper + feature iterator API. |
| c/sedona-gdal/src/vector/feature.rs | New OGR feature + field/geometry helper wrappers. |
| c/sedona-gdal/src/vector.rs | Export newly added vector modules. |
| c/sedona-gdal/src/raster/rasterband.rs | New raster band wrapper (read/write, nodata) + tests. |
| c/sedona-gdal/src/raster.rs | Export raster band module. |
| c/sedona-gdal/src/lib.rs | Expose dataset and driver modules at crate root. |
| c/sedona-gdal/src/errors.rs | Add BufferSizeMismatch error used by raster writes. |
| c/sedona-gdal/src/driver.rs | New driver wrapper + driver lookup + dataset creation helpers + tests. |
| c/sedona-gdal/src/dataset.rs | New dataset wrapper (open, geo/proj, SRS, copy, layer creation, add-band) + tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you!
The vector bits seem lightly tested or not tested...unless you need them for something specific I think we should just port them separately (perhaps more closely targeted to exactly what we need to do with them).
| /// A GDAL dataset. | ||
| pub struct Dataset { | ||
| api: &'static GdalApi, | ||
| c_dataset: GDALDatasetH, | ||
| owned: bool, | ||
| } |
There was a problem hiding this comment.
Can you expand a little on the ownership here and why we need a non-owned version of this? (I trust that you need this, I have just not seen an owned flag that behaves in this way before).
| /// Fetch the projection definition string for this dataset. | ||
| /// Return an empty string if no projection is available. | ||
| pub fn projection(&self) -> String { | ||
| unsafe { | ||
| let ptr = call_gdal_api!(self.api, GDALGetProjectionRef, self.c_dataset); | ||
| if ptr.is_null() { | ||
| String::new() | ||
| } else { | ||
| CStr::from_ptr(ptr).to_string_lossy().into_owned() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Set the projection definition string for this dataset. | ||
| pub fn set_projection(&self, projection: &str) -> Result<()> { | ||
| let c_projection = CString::new(projection)?; | ||
| let rv = unsafe { | ||
| call_gdal_api!( | ||
| self.api, | ||
| GDALSetProjection, | ||
| self.c_dataset, | ||
| c_projection.as_ptr() | ||
| ) | ||
| }; | ||
| if rv != CE_None { | ||
| return Err(self.api.last_cpl_err(rv as u32)); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Given we have a wrapper for SpatialRef, do we need these?
| mod tests { | ||
| use crate::driver::DriverManager; | ||
| use crate::global::with_global_gdal_api; | ||
|
|
There was a problem hiding this comment.
These tests don't cover any of the vector layer features. Should those features be removed for now (I can work on GDAL vector layer stuff later so we can use it to back a datasource and add the associated tests then)
| /// An OGR layer (borrowed from a Dataset). | ||
| pub struct Layer<'a> { | ||
| api: &'static GdalApi, | ||
| c_layer: OGRLayerH, | ||
| _dataset: PhantomData<&'a Dataset>, | ||
| } |
There was a problem hiding this comment.
Unless you need this for the raster work I think we should remove it for now. The options exposed by georust/gdal are not really sufficient for what we would need to do with them to make use of this (and we need a different ownership model so that we can return Box<dyn RecordBatchReader>). I'm happy to port over the vector stuff later.
| /// An OGR feature. | ||
| pub struct Feature<'a> { | ||
| api: &'static GdalApi, | ||
| c_feature: OGRFeatureH, | ||
| _lifetime: PhantomData<&'a ()>, | ||
| } |
There was a problem hiding this comment.
Unless you need this for raster work I think we can port this separately as well. We would almost certainly use the Arrow interface instead of the feature interface.
Summary
Stack
mainvia feat(sedona-gdal): add foundational wrapper utilities #696 and feat(sedona-gdal): add geometry and spatial ref primitives #695mainTesting
cargo test -p sedona-gdalcargo clippy -p sedona-gdal -- -D warnings